Fix #443: tensor symbol identity includes dim and rank - #458
Merged
Merged
Conversation
A tensor symbol compared equal to any symbol of the same name, so A(3,2) and A(2,2) shared one key: a map held one entry for three shapes, and an evaluator holding both bindings served whichever it stored first. Equality and ordering now compare the shape after the name, via a requires-guarded helper so scalar symbols (which carry no shape) are unaffected. The hash is deliberately unchanged — it stays a name-only fast reject, so hash-driven print order and the tensor_scalar_mul / tensor_pow hash invariants are untouched. Signed-off-by: petlenz <[email protected]>
A(3,2) * A(3,4) collapsed to pow(A,2) while symbol identity ignored shape. Tensor multiplication validates dim only, so the like-term path is reachable and deserves its own lock-in. Signed-off-by: petlenz <[email protected]>
symbol_base had to detect dim() and rank() to compare them, so the base of every symbol knew what only a tensor carries. tensor now defines its own comparison operators; equals_same_type casts to the derived type first, so they win by exact match and every path reaches them. Signed-off-by: petlenz <[email protected]>
Signed-off-by: petlenz <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #443.
A tensor symbol's identity covered only its name, so the same name at another shape was the same symbol:
A(3,2) == A(2,2)A(3,2),A(2,2),A(3,4)A(3,2)→I andA(2,2)→7·I, thenapply(A(3,2))trace(A(2,2))with the 2×2 bindingChange.
symbol_base'soperator==/operator<compare the shape after the name, through arequires-guardeddetail::symbol_shapehelper that yields{dim, rank}for symbols whose base carries them and{0,0}otherwise. Scalar symbols are unaffected.Extending
symbol_baserather than overriding intensorkeeps symbol identity in one place: an override would have to restate the name and hash rules, and the last change to them (#348's name tiebreak) would then have needed mirroring in two files — the maintenance hazard behind this bug family.The hash is deliberately untouched. It remains a name-only fast reject, so
A(3,2)andA(2,2)still hash alike and are separated by the full comparison. Verified unchanged: hash-driven print order (pow(x,2)*y*z*X),tensor_scalar_mul(const,T).hash == T.hash,tensor_pow(T,const).hash == T.hash, and all 27 hash tests.Tests (
SymbolIdentity, beside the #348 cases):TensorShapeDistinguishesSameNamecovers the four probes plus ordering totality, incomparability of equal symbols, and a 3-entry map;EvaluatorKeepsBindingsPerTensorShapebinds both shapes in onetensor_evaluatorand asserts each returns its own data, with atracecheck through the t2s evaluator.Negative control: against main's
symbol_base.hboth tests fail (restored withgit show, rebuilt, fresh binary) in Debug and Release; both pass with the fix. Unlike the sibling #348, Release does not crash here — the evaluator dispatches on the bound data's own shape, so the failure is silently wrong numbers.Suites: Debug 2407/2407, Release 2407/2407 (gcc-14,
-Werrorminus the pedantic/deprecated-declarations warnings already on main). clang-format-18 clean.Scope notes. Scalar symbols carry no shape-like state, and there is no t2s symbol type (t2s wraps a scalar expression), so nothing analogous is excluded elsewhere. The parser was never exposed:
symbol_tablealready raisesredeclaration_errorwhen a name reappears with a different(rank, dim)— this was reachable only through the C++ API.Review finding — a second silent wrong result this also fixes.
A(3,2) * A(3,4)collapsed topow(A,2)on main: a rank-2 and a rank-4 tensor merged as one factor, because the like-term path compared symbols by name only. With shape in the identity it correctly staysA*A. Tensor multiplication validates dim but not rank (#438 covers+/-), so that path stays reachable — now pinned bySymbolIdentity.MixedRankFactorsDoNotMerge, which fails against main's identity.Review fix — shape is compared where the shape lives. The first version put the comparison in
symbol_base, which had to detectdim()/rank()with arequiresclause: the base of every symbol then knew about state only a tensor carries.tensornow defines its own==,!=,<and>;equals_same_type/less_than_same_typecast to the derived type before comparing, so these win by exact match and every path reaches them.symbol_baseis back to name-only identity with a comment pointing at the extension mechanism.No virtual was needed: the dispatch already goes through the derived type. Negative control: with
tensor.hrestored from main, the three shape tests fail while the two cross-domain tests still pass — confirming the shape logic now lives entirely intensor. Suite 2408/2408.